The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

Unlinked Accounts

The following contributors have not linked their GitHub and WordPress.org accounts: @apeatling.

Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Unlinked contributors: apeatling.

Co-authored-by: im3dabasia <im3dabasia1@git.wordpress.org>
Co-authored-by: aaronrobertshaw <aaronrobertshaw@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>
Co-authored-by: mtias <matveb@git.wordpress.org>
Co-authored-by: annezazu <annezazu@git.wordpress.org>
Co-authored-by: markhowellsmead <markhowellsmead@git.wordpress.org>
Co-authored-by: bacoords <bacoords@git.wordpress.org>
Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

I think the better fix is to make onSelect and onDeselect fire on actual item menu selection transitions:

One thing we might want to keep an eye on or add tests for is ensuring we aren't unnecessarily firing this onDeselect in terms of resetting values.

One thing we might want to keep an eye on or add tests for is ensuring we aren't unnecessarily firing this onDeselect in terms of resetting values.

Yes, exactly that. When setting aspect ratio to "Original", it programmatically hides the scale item, which now also triggers onDeselect on scale, causing onChange to fire a second time.

The isValueSet gate was preventing this, but removing it to fix the no-value case broke this scenario. The fix needs to distinguish user-initiated toggles from programmatic ones, happy to explore that if you can point me to the right place to thread that through context.

This is something I'd need to spend some extra time digging into to give you an exact look here. Unfortunately, I'm a little preoccupied on other work at the moment before I'll be away for a couple of weeks.

My initial thought was to maybe pass the isValueSet through to the onDeselect however that would require all consumers to update their onDeselect callbacks.

Maybe we need onShow and onHide handlers. The onShow could be an alias to onSelect perhaps. The onHide gets called regardless of isValueSet whereas onDeselect is still value gated. None of this sounds great at the moment but hopefully it helps move the discussion forward.

Something else worth discussing could be changing the public API such that onDeselect is renamed to onReset.

Whatever we do we'll need to maintain backward compatibility as well as keep this visibility tracking and management all at the same level, the item level.

@aaronrobertshaw , Thanks for the quick responses.

What if we track whether a menu item was toggled by the user, and use that to gate onDeselect? This way it only fires on genuine user actions. Internal/programmatic state changes won't cause double calls.

It's a small change, maintains backward compatibility, and requires very few architectural modifications. Happy to explore further based on your feedback!

I may not have much context, but could we migrate to a typical controlled/uncontroleld set of props to manage each ToolsPanel.Item "visibility" ? Ie. shown / defaultShown / onShownChange (name TBD)

Today's behaviour is uncontrolled, and could be kept the same (initial value set via defaultShown). A controlled approach would instead use shown and onShownChange.

Could that work?